Skip to content

Conversation

@Nils-ChristianIseke
Copy link
Contributor

@Nils-ChristianIseke Nils-ChristianIseke commented Mar 21, 2025

Regarding #529.
This is just a first draft for a README (Far from beeing perfect).
I would appreciate some feedback regarding the points below.

This PR proposes an initial README to make the repository more self-contained. While minimal, it aims to:

  • Clarify ament_lint’s purpose for new contributors/users.
  • Explain key benefits /shortcoming over manual linting setups.
  • Provide quick-start examples for common workflows (e.g. by refering the tools docs)

I still have the following open questions which might be answered by the readme:

  • Why Not Just Share Tool Configs?
    If the goal is to standardize linting, why bundle configurations with CMake integration instead of distributing standalone configs (e.g., .clang-format, flake8.ini) that developers could adopt via other tools like pre-commit?

  • Abstraction Tradeoffs
    While CMake integration benefits some workflows, it adds a layer that complicates debugging or customization for others. For example, developers using pre-commit might see ament_lint as more of a burdon.

  • Could ament_lint focus on integration while decoupling from configuration?

  • ROS Core Team’s Reasoning, e.g:
    Ament_lint is the officially supported ROS 2 solution for linting because it ...

Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
@Nils-ChristianIseke Nils-ChristianIseke marked this pull request as ready for review March 24, 2025 21:55
@christophebedard christophebedard self-requested a review April 3, 2025 17:20
@christophebedard christophebedard self-assigned this Apr 3, 2025
@christophebedard
Copy link
Member

I don't want to pretend that I know all the answers, but I can try to share what I know.

Provide quick-start examples for common workflows (e.g. by refering the tools docs)

You could just point to each tool's documentation, see my other comment.

  • Why Not Just Share Tool Configs?
    If the goal is to standardize linting, why bundle configurations with CMake integration instead of distributing standalone configs (e.g., .clang-format, flake8.ini) that developers could adopt via other tools like pre-commit?

I think it's just easier, especially for the ROS 2 core. And, for other packages, I think the tools matter more than their default configuration file, since that often depends on preferences.

Also, you can run these tools yourself through the CLI, so they can be integrated into pre-commit hooks.

  • Abstraction Tradeoffs
    While CMake integration benefits some workflows, it adds a layer that complicates debugging or customization for others. For example, developers using pre-commit might see ament_lint as more of a burden.

With ament_lint tools, the linter is wrapped into an ament_$linter CLI tool, which is then wrapped in CMake (ament_cmake_$linter) or called in a minimal Python file that then becomes a "normal" test, for example: https://github.com/ros2/ros2_tracing/blob/rolling/tracetools_trace/test/test_mypy.py. It is definitely just one approach, and other people have also pointed this out before: linters are usually separate from tests. And using standalone linters often makes setting up/maintaining pre-commit hooks easier.

  • Could ament_lint focus on integration while decoupling from configuration?

Possibly, but there hasn't been a good reason to do so yet, or at least a reason that would make the additional complexity/maintenance worth it.

  • ROS Core Team’s Reasoning, e.g:
    Ament_lint is the officially supported ROS 2 solution for linting because it ...

The answer to this is probably partly historical. (As an aside, you might wonder: "why is it called "ament?" It's called ament because the ROS 1 equivalent(ish) of this toolset/buildsystem layer was called catkin, another name for the same flower. But why was that called "catkin?" It goes on and on 😄)

I wouldn't necessarily say it's the "officially supported ROS 2 solution for lining;" it's just the most common/most well-integrated linting solution we have. As for why that is, I'd say ROS 2 converged to this solution because of what I mentioned above. Overall, given that ROS 2 is made up of a bunch of repos, it's easier to set up linters as normal tests instead of commit hooks.

Signed-off-by: Nils-Christian Iseke <[email protected]>
@Nils-ChristianIseke Nils-ChristianIseke marked this pull request as draft April 14, 2025 19:25
## ament_lint

ament_lint contains a number of packages, each package wraps a linter into an ament_$linter CLI tool, which is then wrapped in CMake (ament_cmake_$linter)
Those linters can be used both from the command line and from CMake.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They also have a Python API, e.g., here:

def main(argv: List[str] = sys.argv[1:]) -> int:
. This can be called in a test, e.g., here: https://github.com/ros2/launch/blob/f8ef67a0f8cface863e1cd168f0b6bf2976be60b/launch/test/test_mypy.py#L22.

@Nils-ChristianIseke
Copy link
Contributor Author

Nils-ChristianIseke commented Apr 17, 2025

Thanks for this detailed reply :).

I'm struggling a bit to write a good summary for these thoughts.
E.g. something like: Ament_lint is for you if you want xyz, you better not use ament_lint if you want xyz. Do you have a suggestion?

I think it's mainly because I don't understand the point of ament_lint. I understand that it's convenient for some developers to have the cmake integration, but I think many developers would also like the ability to apply the Ros core coding styles via linters without being forced to maintain their own configuration and/or use ament_lint. Personally, for example, I would just use pre-commit and run the pure linters. Maybe we can think about providing a pre-commit configruation that ros2 users can use as alternatives to the wrapped linters?. Am I missing something with this approach?

@christophebedard
Copy link
Member

I don't think there's any deep reason for the way it is. It's just one solution, and it's the one we have. It's used by the ROS 2 core, and it can be used by other users if they want to use it.

In some cases, the ament_[cmake_]* wrapper also makes sure to create a standard xUnit result file:

  • set(result_file "${AMENT_TEST_RESULTS_DIR}/${PROJECT_NAME}/${ARG_TESTNAME}.xunit.xml")
    set(cmd "${ament_uncrustify_BIN}" "--xunit-file" "${result_file}")
  • # generate xunit file
    if args.xunit_file:
    folder_name = os.path.basename(os.path.dirname(args.xunit_file))
    file_name = os.path.basename(args.xunit_file)
    suffix = '.xml'
    if file_name.endswith(suffix):
    file_name = file_name[0:-len(suffix)]
    suffix = '.xunit'
    if file_name.endswith(suffix):
    file_name = file_name[0:-len(suffix)]
    testname = '%s.%s' % (folder_name, file_name)
    xml = get_xunit_content(report, testname, time.time() - start_time)
    path = os.path.dirname(os.path.abspath(args.xunit_file))
    if not os.path.exists(path):
    os.makedirs(path)
    with open(args.xunit_file, 'w') as f:
    f.write(xml)

but, again, this is because these tools are primarily used through tests (see test_mypy.py in my comment above), which a lot of people don't like, even if you can run ament_mypy directly in a terminal (or in CI). You could mention that.

Maybe we can think about providing a pre-commit configruation that ros2 users can use as alternatives to the wrapped linters?. Am I missing something with this approach?

I think that's probably outside of the scope of this repo, but people can do it if they want (and I know some have done it). One reason it's not done for the ROS 2 core itself is that there are so many repos and it would be annoying to set up hooks and maintain them.

but I think many developers would also like the ability to apply the Ros core coding styles via linters without being forced to maintain their own configuration and/or use ament_lint.

I mean, you could just use the configuration file used by the corresponding ament_* tool, e.g.:

You could fetch these via curl or whatever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants